feat: add idempotency checks for third-party dependencies#413
feat: add idempotency checks for third-party dependencies#41313eholder wants to merge 1 commit intoeloqdata:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded an idempotency helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/install_dependency_ubuntu2404.sh (2)
19-27: Avoidevalinis_installed; execute probes as argv.Using
evalhere is unnecessary and riskier than direct command invocation. All five call sites use simple command probes suitable for conversion. Refactor the function to use"$@"instead, then update each call site to pass the command as separate arguments:Refactoring pattern
is_installed() { - if eval "$2" >/dev/null 2>&1; then - echo "--- [SKIP] $1 is already installed. ---" + local library_name="$1" + shift + if "$@" >/dev/null 2>&1; then + echo "--- [SKIP] ${library_name} is already installed. ---" return 0 fi return 1 }Apply to all five call sites (lines 65, 75, 85, 100, 120), e.g.:
-if ! is_installed "glog" "ls /usr/local/lib/libglog.so"; then +if ! is_installed "glog" test -f /usr/local/lib/libglog.so; thenNote:
test -fis more efficient thanlsfor existence checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` around lines 19 - 27, The is_installed function currently uses eval to run the probe command; change it to accept the probe as argv (e.g., use "$@" to run the passed command) and update every call site that invokes is_installed to pass the command and its arguments as separate parameters instead of a single string (convert probes like "ls /usr/include/some.h" to is_installed "SomeLib" test -f /usr/include/some.h or similar); replace any uses of ls with test -f for file existence checks and ensure return behavior and logging (echo "--- [SKIP] $1 is already installed. ---") remain unchanged in is_installed so callers keep the same flow.
75-82: Use pkg-config or ldconfig instead of hardcoded path for liburing detection.Line 75 hardcodes
/usr/lib/liburing.so.2.6, which is fragile across install layouts and targets the wrong version identifier. liburing's SONAME isliburing.so.2(not.2.6), and the path doesn't account for/usr/lib64, multiarch paths, or/usr/local/lib. Instead:
- Build-time: Use
pkg-config --exists liburing(liburing installsliburing.pc)- Runtime: Use
ldconfig -p | grep liburing.so.2to probe the dynamic linker cache🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` around lines 75 - 82, The current install check in scripts/install_dependency_ubuntu2404.sh uses a hardcoded path "/usr/lib/liburing.so.2.6" which is fragile; update the detection logic around the is_installed call to first check build-time availability with "pkg-config --exists liburing" (use liburing.pc), and for runtime probe use "ldconfig -p | grep liburing.so.2" to detect the SONAME across /usr/lib, /usr/lib64, /usr/local/lib and multiarch layouts; change the condition that calls is_installed (or replace it) to return true if either pkg-config succeeds or ldconfig finds liburing.so.2, otherwise proceed with the git clone/build steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 19-27: The current is_installed() helper only tests presence and
must be made version-aware: change is_installed to accept a required-version
argument and, for each dependency (liburing, aws-sdk-cpp, Catch2), run a version
query (e.g. pkg-config --modversion, dpkg-query -W -f='${Version}', or cmake
--find-package with VERSION) and compare the installed version against the
required pin (liburing-2.6, aws-sdk-cpp 1.11.446, Catch2 3.3.2); update all
invocations of is_installed to pass the expected version and only skip
installation when the installed version >= required version, otherwise return
non-zero so the install path runs. Ensure the function logs both detected and
required versions in its messages for clarity.
---
Nitpick comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 19-27: The is_installed function currently uses eval to run the
probe command; change it to accept the probe as argv (e.g., use "$@" to run the
passed command) and update every call site that invokes is_installed to pass the
command and its arguments as separate parameters instead of a single string
(convert probes like "ls /usr/include/some.h" to is_installed "SomeLib" test -f
/usr/include/some.h or similar); replace any uses of ls with test -f for file
existence checks and ensure return behavior and logging (echo "--- [SKIP] $1 is
already installed. ---") remain unchanged in is_installed so callers keep the
same flow.
- Around line 75-82: The current install check in
scripts/install_dependency_ubuntu2404.sh uses a hardcoded path
"/usr/lib/liburing.so.2.6" which is fragile; update the detection logic around
the is_installed call to first check build-time availability with "pkg-config
--exists liburing" (use liburing.pc), and for runtime probe use "ldconfig -p |
grep liburing.so.2" to detect the SONAME across /usr/lib, /usr/lib64,
/usr/local/lib and multiarch layouts; change the condition that calls
is_installed (or replace it) to return true if either pkg-config succeeds or
ldconfig finds liburing.so.2, otherwise proceed with the git clone/build steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d0410b7-c293-482d-987d-6db6e1ad76cb
📒 Files selected for processing (1)
scripts/install_dependency_ubuntu2404.sh
| # 参数1: 库名(用于显示) | ||
| # 参数2: 检测命令 (检测核心头文件或库文件是否存在) | ||
| is_installed() { | ||
| if eval "$2" >/dev/null 2>&1; then | ||
| echo "--- [SKIP] $1 is already installed. ---" | ||
| return 0 | ||
| fi | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Pinned versions / tags in installer:"
rg -n 'git checkout tags/|git clone -b v|is_installed' scripts/install_dependency_ubuntu2404.sh -C1
echo
echo "CMake dependency resolution without explicit versions:"
rg -n 'find_package\((glog|AWSSDK|Catch2)\b' CMakeLists.txt tests/CMakeLists.txtRepository: eloqdata/eloqstore
Length of output: 1410
🏁 Script executed:
sed -n '50,65p' CMakeLists.txtRepository: eloqdata/eloqstore
Length of output: 432
🏁 Script executed:
sed -n '1,10p' tests/CMakeLists.txtRepository: eloqdata/eloqstore
Length of output: 253
Installation skip logic should validate dependency versions, not just presence.
Line 22 checks only whether a dependency is installed, but the script pins specific versions (liburing-2.6, aws-sdk-cpp 1.11.446, Catch2 v3.3.2) while CMake resolution accepts any version (find_package(glog/AWSSDK/Catch2 REQUIRED) without VERSION constraints). This allows stale or incompatible system versions to be skipped, potentially causing build inconsistencies or runtime failures.
🔧 Suggested approach (version-aware skip)
is_installed() {
- if eval "$2" >/dev/null 2>&1; then
+ local name="$1"
+ local probe_cmd="$2"
+ local version_cmd="${3:-}"
+ local expected_version="${4:-}"
+ if eval "$probe_cmd" >/dev/null 2>&1; then
+ if [ -n "$version_cmd" ] && [ -n "$expected_version" ]; then
+ local actual_version
+ actual_version="$(eval "$version_cmd" 2>/dev/null || true)"
+ if [ "$actual_version" != "$expected_version" ]; then
+ echo "--- [REINSTALL] ${name}: ${actual_version} != ${expected_version} ---"
+ return 1
+ fi
+ fi
- echo "--- [SKIP] $1 is already installed. ---"
+ echo "--- [SKIP] ${name} is already installed. ---"
return 0
fi
return 1
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 参数1: 库名(用于显示) | |
| # 参数2: 检测命令 (检测核心头文件或库文件是否存在) | |
| is_installed() { | |
| if eval "$2" >/dev/null 2>&1; then | |
| echo "--- [SKIP] $1 is already installed. ---" | |
| return 0 | |
| fi | |
| return 1 | |
| } | |
| # 参数1: 库名(用于显示) | |
| # 参数2: 检测命令 (检测核心头文件或库文件是否存在) | |
| is_installed() { | |
| local name="$1" | |
| local probe_cmd="$2" | |
| local version_cmd="${3:-}" | |
| local expected_version="${4:-}" | |
| if eval "$probe_cmd" >/dev/null 2>&1; then | |
| if [ -n "$version_cmd" ] && [ -n "$expected_version" ]; then | |
| local actual_version | |
| actual_version="$(eval "$version_cmd" 2>/dev/null || true)" | |
| if [ "$actual_version" != "$expected_version" ]; then | |
| echo "--- [REINSTALL] ${name}: ${actual_version} != ${expected_version} ---" | |
| return 1 | |
| fi | |
| fi | |
| echo "--- [SKIP] ${name} is already installed. ---" | |
| return 0 | |
| fi | |
| return 1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install_dependency_ubuntu2404.sh` around lines 19 - 27, The current
is_installed() helper only tests presence and must be made version-aware: change
is_installed to accept a required-version argument and, for each dependency
(liburing, aws-sdk-cpp, Catch2), run a version query (e.g. pkg-config
--modversion, dpkg-query -W -f='${Version}', or cmake --find-package with
VERSION) and compare the installed version against the required pin
(liburing-2.6, aws-sdk-cpp 1.11.446, Catch2 3.3.2); update all invocations of
is_installed to pass the expected version and only skip installation when the
installed version >= required version, otherwise return non-zero so the install
path runs. Ensure the function logs both detected and required versions in its
messages for clarity.
13eholder
force-pushed
the
feat/opt_install_dependency
branch
from
afeab49 to
c911ad0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/install_dependency_ubuntu2404.sh (1)
85-97:⚠️ Potential issue | 🟡 MinorPin brpc to a specific tag or commit.
Similar to glog, brpc is cloned without a version pin, making builds non-reproducible and vulnerable to upstream breakage.
🔧 Suggested fix
if ! is_installed "brpc" "ls /usr/lib/libbrpc.so"; then -git clone https://github.com/eloqdata/brpc.git brpc +git clone --branch <pinned-tag-or-commit> https://github.com/eloqdata/brpc.git brpc cd brpc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` around lines 85 - 97, The brpc clone step is unpinned which makes builds non-reproducible; update the block that clones and builds brpc (the if ! is_installed "brpc" "ls /usr/lib/libbrpc.so" branch) to checkout a specific tag or commit after git clone (e.g., run git fetch && git checkout <TAG_OR_COMMIT>) before building, and document the chosen tag/commit; ensure the rest of the steps (cmake, cmake --build, and sudo cp ...) remain unchanged so the pinned revision is built instead of the repository tip.
🧹 Nitpick comments (1)
scripts/install_dependency_ubuntu2404.sh (1)
75-82: Quote command substitution to prevent word splitting.Per shellcheck SC2046, quote
$(nproc)on line 80.🔧 Suggested fix
- make -j$(nproc) && sudo make install + make -j "$(nproc)" && sudo make install🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` around lines 75 - 82, The make invocation uses unquoted command substitution which can cause word-splitting (SC2046); update the build step in the liburing install block (the make -j$(nproc) call inside the if checking is_installed) to quote the substitution, i.e., use "$(nproc)" so the command becomes make -j"$(nproc)"; ensure only that substitution is quoted and no other behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 65-72: The glog installation block (guarded by is_installed)
clones the repo at HEAD which is not pinned; change the git clone to fetch a
specific tag or commit (e.g., git clone --branch <TAG_OR_COMMIT> --depth 1
https://github.com/eloqdata/glog.git glog or git -C glog checkout <COMMIT>) so
builds are reproducible, and also quote the nproc substitution in the cmake
build call (use "-j$(nproc)" -> "-j\"$(nproc)\"" or simply "-j\"$(nproc)\"" in
the cmake --build invocation) to avoid word-splitting; update the glog cleanup
(cd ../ && rm -rf glog) only after successful install in the same block.
---
Outside diff comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 85-97: The brpc clone step is unpinned which makes builds
non-reproducible; update the block that clones and builds brpc (the if !
is_installed "brpc" "ls /usr/lib/libbrpc.so" branch) to checkout a specific tag
or commit after git clone (e.g., run git fetch && git checkout <TAG_OR_COMMIT>)
before building, and document the chosen tag/commit; ensure the rest of the
steps (cmake, cmake --build, and sudo cp ...) remain unchanged so the pinned
revision is built instead of the repository tip.
---
Nitpick comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 75-82: The make invocation uses unquoted command substitution
which can cause word-splitting (SC2046); update the build step in the liburing
install block (the make -j$(nproc) call inside the if checking is_installed) to
quote the substitution, i.e., use "$(nproc)" so the command becomes make
-j"$(nproc)"; ensure only that substitution is quoted and no other behavior
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e7101ff-58a1-45f0-b957-5bfe58545418
📒 Files selected for processing (1)
scripts/install_dependency_ubuntu2404.sh
| if ! is_installed "glog" "ls /usr/local/lib/libglog.so"; then | ||
| git clone https://github.com/eloqdata/glog.git glog | ||
| cd glog | ||
| cmake -S . -B build -G "Unix Makefiles" | ||
| cmake --build build -j$(nproc) | ||
| sudo cmake --build build --target install | ||
| cd ../ && rm -rf glog | ||
| fi |
There was a problem hiding this comment.
Pin glog to a specific tag or commit for reproducible builds.
Unlike liburing, AWS SDK, and Catch2 which specify exact versions, glog is cloned from HEAD of the default branch. Upstream changes could break builds without notice.
Additionally, quote $(nproc) to avoid word splitting (shellcheck SC2046).
🔧 Suggested fix
if ! is_installed "glog" "ls /usr/local/lib/libglog.so"; then
- git clone https://github.com/eloqdata/glog.git glog
+ git clone --branch <pinned-tag-or-commit> https://github.com/eloqdata/glog.git glog
cd glog
cmake -S . -B build -G "Unix Makefiles"
- cmake --build build -j$(nproc)
+ cmake --build build -j "$(nproc)"
sudo cmake --build build --target install
cd ../ && rm -rf glog
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 69-69: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install_dependency_ubuntu2404.sh` around lines 65 - 72, The glog
installation block (guarded by is_installed) clones the repo at HEAD which is
not pinned; change the git clone to fetch a specific tag or commit (e.g., git
clone --branch <TAG_OR_COMMIT> --depth 1 https://github.com/eloqdata/glog.git
glog or git -C glog checkout <COMMIT>) so builds are reproducible, and also
quote the nproc substitution in the cmake build call (use "-j$(nproc)" ->
"-j\"$(nproc)\"" or simply "-j\"$(nproc)\"" in the cmake --build invocation) to
avoid word-splitting; update the glog cleanup (cd ../ && rm -rf glog) only after
successful install in the same block.
|
|
||
| # 参数1: 库名(用于显示) | ||
| # 参数2: 检测命令 (检测核心头文件或库文件是否存在) | ||
| is_installed() { |
There was a problem hiding this comment.
Dependency libs may be updated. The idempotency check should be enabled by a parameter which is by default false.
13eholder
force-pushed
the
feat/opt_install_dependency
branch
from
c911ad0 to
79cb862
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 85-86: The brpc clone is unpinned (git clone
https://github.com/eloqdata/brpc.git brpc) causing non-reproducible builds;
update the install block that checks is_installed("brpc", ...) so after cloning
the repo you either checkout a specific tag/commit (e.g., run git -C brpc
checkout tags/<VERSION> or git -C brpc checkout <COMMIT_HASH>) or change the
clone to pin a branch/tag (e.g., clone with --branch <TAG> --depth 1) to match
how liburing/AWS SDK/Catch2 are pinned; ensure the chosen <VERSION> or commit is
stored as the canonical value used elsewhere in the script.
- Line 80: The make invocation uses an unquoted command substitution "make
-j$(nproc)" which triggers ShellCheck SC2046; update the script to quote the
substitution so it becomes make -j"$(nproc)" to prevent potential
word-splitting/globbing issues and satisfy shell safety best practices, leaving
the rest of the line (&& sudo make install) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 559fcd85-9782-4bee-98b2-1f8e93b0ecfc
📒 Files selected for processing (1)
scripts/install_dependency_ubuntu2404.sh
| cd liburing | ||
| git checkout tags/liburing-2.6 | ||
| ./configure --cc=gcc --cxx=g++ | ||
| make -j$(nproc) && sudo make install |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nF '-j$(nproc)' scripts/install_dependency_ubuntu2404.shRepository: eloqdata/eloqstore
Length of output: 91
🏁 Script executed:
#!/bin/bash
# First, let's see if the file exists and how many lines it has
wc -l scripts/install_dependency_ubuntu2404.sh
# Read lines around 80 and 115 to verify the pattern
echo "=== Lines 75-85 ==="
sed -n '75,85p' scripts/install_dependency_ubuntu2404.sh
echo "=== Lines 110-120 ==="
sed -n '110,120p' scripts/install_dependency_ubuntu2404.sh
# Search for all occurrences of -j$ pattern
echo "=== All -j\$ patterns ==="
grep -n '\-j$' scripts/install_dependency_ubuntu2404.sh || echo "Pattern not found with literal $"
# Try searching without escape
grep -n 'j.*nproc' scripts/install_dependency_ubuntu2404.shRepository: eloqdata/eloqstore
Length of output: 1120
🏁 Script executed:
#!/bin/bash
# Check if ShellCheck is available and what it says about this pattern
which shellcheck > /dev/null 2>&1 && {
echo "=== ShellCheck output for line 80 context ==="
shellcheck -S style scripts/install_dependency_ubuntu2404.sh 2>&1 | grep -A2 -B2 "nproc" || echo "No shellcheck issues found or tool not available"
} || echo "ShellCheck not available"
# Verify the exact pattern - all -j$(nproc) occurrences
echo "=== All -j\$(nproc) occurrences ==="
grep -n '\-j\$(' scripts/install_dependency_ubuntu2404.shRepository: eloqdata/eloqstore
Length of output: 292
🌐 Web query:
ShellCheck SC2046 unquoted command substitution rule explanation
💡 Result:
ShellCheck SC2046 (“Quote this to prevent word splitting”) warns when you use an unquoted command substitution like:
ls -l $(getfilename)Because after $(...) is expanded, the shell will still apply word splitting (on spaces/tabs/newlines via $IFS) and globbing (* ? [...]). So if the command outputs something like My File.txt, it may turn into two arguments (My and File.txt) or even expand wildcards unexpectedly. The usual fix is to quote the substitution so it becomes exactly one argument:
ls -l "$(getfilename)"[1][2]
If the command substitution is meant to produce multiple items, don’t rely on splitting $(...); use a loop or read into an array (e.g., read -ra args / readarray) and then expand safely (e.g., "${args[@]}"). [1]
Exception: in rare cases you do want word splitting (e.g., gcc $(pkg-config --libs openssl) ...), and ShellCheck suggests either disabling SC2046 for that line or using an array-based approach instead. [1]
Sources: ShellCheck SC2046 wiki [1]; Greg’s Wiki on quoting, word splitting, and globbing [2].
🏁 Script executed:
#!/bin/bash
# Test if quoted -j flag works with make and cmake
# We can check the man pages or documentation snippets
# Check make documentation format for -j
man make 2>/dev/null | grep -A5 '\-j' | head -20 || echo "man page not available"
# Verify syntax: both make and cmake should accept -j with quoted numeric values
# The key is whether "-j8" and -j"8" are parsed identically
echo "=== Checking if this is a real issue ==="
echo "$(nproc) returns a single number, e.g., 8"
echo "So -j\$(nproc) becomes -j8 (unquoted)"
echo "And -j\"\$(nproc)\" becomes -j\"8\" (quoted)"
echo "Both should be parsed identically by make/cmake as the -j flag with argument 8"Repository: eloqdata/eloqstore
Length of output: 354
Quote $(nproc) to satisfy ShellCheck SC2046 and follow shell safety best practices.
ShellCheck flags unquoted command substitutions to prevent word-splitting and globbing. While $(nproc) returns a single numeric value with minimal practical risk, quoting it as "$(nproc)" follows best practice for shell safety and consistency. The -j"8" and -j8 syntaxes are functionally equivalent for make and cmake.
Suggested fix
- make -j$(nproc) && sudo make install
+ make -j"$(nproc)" && sudo make install
...
- cmake --build . --config RelWithDebInfo -j$(nproc)
+ cmake --build . --config RelWithDebInfo -j"$(nproc)"🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 80-80: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install_dependency_ubuntu2404.sh` at line 80, The make invocation
uses an unquoted command substitution "make -j$(nproc)" which triggers
ShellCheck SC2046; update the script to quote the substitution so it becomes
make -j"$(nproc)" to prevent potential word-splitting/globbing issues and
satisfy shell safety best practices, leaving the rest of the line (&& sudo make
install) unchanged.
79cb862 to
9b8d901
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/install_dependency_ubuntu2404.sh (1)
146-158: Good version-aware detection for Catch2; inconsistent parallelism.The
pkg-config --exact-versioncheck is a clean approach for version validation.However, line 155 uses hardcoded
-j4instead of-j"$(nproc)", which is inconsistent with all other build commands in this script and may underutilize available cores on larger machines.Suggested fix
- cmake --build . -j4 + cmake --build . -j"$(nproc)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` around lines 146 - 158, Replace the hardcoded parallel build flag in the Catch2 installation block to use available CPU cores; specifically, in the section that checks CATCH2_CHECK_CMD and then builds Catch2 (the cmake --build invocation inside the Catch2 bld directory), change the build parallelism from the fixed -j4 to use -j"$(nproc)" so it matches other build commands in the script and scales with the machine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 21-27: The for-loop over "for arg in \"$@\"" contains an
ineffective shift; remove the dead shift or convert the loop to a
positional-parameter while-loop so shifting actually changes arguments. Replace
the current loop with either (a) remove the single "shift" inside the for-loop
and just set CHECK_IDEMPOTENCY=true in the case branch, or (b) change to "while
[ $# -gt 0 ]; do case \"$1\" in --check) CHECK_IDEMPOTENCY=true; shift;; esac;
done" so shifts operate on $1 and subsequent cases work; update references to
CHECK_IDEMPOTENCY accordingly.
---
Nitpick comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 146-158: Replace the hardcoded parallel build flag in the Catch2
installation block to use available CPU cores; specifically, in the section that
checks CATCH2_CHECK_CMD and then builds Catch2 (the cmake --build invocation
inside the Catch2 bld directory), change the build parallelism from the fixed
-j4 to use -j"$(nproc)" so it matches other build commands in the script and
scales with the machine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 780c6c38-fa08-424f-8165-b0788a5b8027
📒 Files selected for processing (2)
README.mdscripts/install_dependency_ubuntu2404.sh
✅ Files skipped from review due to trivial changes (1)
- README.md
13eholder
force-pushed
the
feat/opt_install_dependency
branch
from
9b8d901 to
ff621cf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/install_dependency_ubuntu2404.sh (3)
144-156: Good version-aware detection withpkg-config --exact-version.The Catch2 installation properly uses
pkg-config --exact-versionfor version-aware idempotency checking.Minor inconsistency: Line 153 uses hardcoded
-j4while other builds use-j"$(nproc)". Consider aligning for consistency:- cmake --build . -j4 + cmake --build . -j"$(nproc)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` around lines 144 - 156, Update the Catch2 build invocation to use the machine CPU count instead of a hardcoded thread count: replace the cmake build call that uses -j4 (within the Catch2 install block referencing CATCH2_VERSION and CATCH2_CHECK_CMD) with -j"$(nproc)" so it matches the other builds' use of nproc for parallelism and keeps consistency across the script.
116-141: Good version-aware detection; quote$(nproc)for shell safety.The AWS SDK version detection via
VersionConfig.hheader checks is well-implemented and addresses version-awareness concerns. However,$(nproc)on line 136 should be quoted per ShellCheck SC2046.- cmake --build . --config RelWithDebInfo -j$(nproc) + cmake --build . --config RelWithDebInfo -j"$(nproc)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` around lines 116 - 141, The build invocation uses unquoted command substitution (-j$(nproc)) which can trigger ShellCheck SC2046; update the cmake build command that contains -j$(nproc) (in the block that clones/builds aws and the AWS_SDK_CHECK_CMD surrounding variables) to quote the substitution as -j"$(nproc)" so the job-count is treated as a single word and avoids word-splitting issues.
24-24: Translate Chinese comment to English for readability.The comment
找到即可退出should be in English to maintain consistency and readability across the codebase.- break # 找到即可退出 + break # Found, exit loop🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_dependency_ubuntu2404.sh` at line 24, Replace the Chinese inline comment next to the break statement (the comment "找到即可退出") with an English equivalent for readability; update the comment adjacent to the break in scripts/install_dependency_ubuntu2404.sh so it reads something like "found, exit loop" or "exit once found" to preserve meaning and context around the break statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install_dependency_ubuntu2404.sh`:
- Around line 144-156: Update the Catch2 build invocation to use the machine CPU
count instead of a hardcoded thread count: replace the cmake build call that
uses -j4 (within the Catch2 install block referencing CATCH2_VERSION and
CATCH2_CHECK_CMD) with -j"$(nproc)" so it matches the other builds' use of nproc
for parallelism and keeps consistency across the script.
- Around line 116-141: The build invocation uses unquoted command substitution
(-j$(nproc)) which can trigger ShellCheck SC2046; update the cmake build command
that contains -j$(nproc) (in the block that clones/builds aws and the
AWS_SDK_CHECK_CMD surrounding variables) to quote the substitution as
-j"$(nproc)" so the job-count is treated as a single word and avoids
word-splitting issues.
- Line 24: Replace the Chinese inline comment next to the break statement (the
comment "找到即可退出") with an English equivalent for readability; update the comment
adjacent to the break in scripts/install_dependency_ubuntu2404.sh so it reads
something like "found, exit loop" or "exit once found" to preserve meaning and
context around the break statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75604408-0218-4354-aee0-9882967b8b1d
📒 Files selected for processing (2)
README.mdscripts/install_dependency_ubuntu2404.sh
✅ Files skipped from review due to trivial changes (1)
- README.md
13eholder
force-pushed
the
feat/opt_install_dependency
branch
from
ff621cf to
b55702e
Compare
| # arg 1: Library name (for display) | ||
| # arg 2: Detection command (to check whether the core header file or library file exists) | ||
| is_installed() { | ||
| if [ "$CHECK_IDEMPOTENCY" = "false" ]; then |
There was a problem hiding this comment.
CHECK_IDEMPOTENCY should not change the meaning of is_installed, change is_installed to need_install or check CHECK_IDEMPOTENCY before installing.
2 participants
Here are some reminders before you submit the pull request
is_installedhelper to skip already installed libraries.fixes eloqdb/eloqstore#314ctest --test-dir build/tests/Summary by CodeRabbit
Chores
Documentation